-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Fleet] Ensure policies are not out of sync #175065
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
/ci |
Pinging @elastic/fleet (Team:Fleet) |
|
||
if (outdatedAgentPolicyIds.length) { | ||
await agentPolicyService.deployPolicies(soClient, outdatedAgentPolicyIds).catch((error) => { | ||
logger.warn(`Error deploying policies: ${error.message}`, { error }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is no need to crash the setup if this fail, it's why I am swallowing the error here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could we add a unit test for the error scenario? I usually combine async-await with try-catch, I suppose it should work with catch too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me - adding an eventual consistency check for policies to Fleet's setup is sensible 👍
// be a bottleneck in environments with several thousand agent policies being deployed here. | ||
(agentPolicyId) => agentPolicyService.getFullAgentPolicy(soClient, agentPolicyId), | ||
{ | ||
concurrency: 50, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
/ci |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
Added a small comment, otherwise LGTM 🚀
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @nchaulet |
Summary
Related to https://github.com/elastic/ingest-dev/issues/2120
Re-introduce
ensureFleetServerAgentPoliciesExists
deleted (by mistake I think) in #121628 .I made some refacto to avoid retrieving multiple time policies to both verify enrollement token and
.fleet-policies
Tests
I added some unit test to cover that, you can manually test that by deleting document in
.fleet-policies
and verifying that the policy is created again on next setupFor example in the devtools